-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(apigatewayv2): incorrect arn function causing unwanted behavior #33100
base: main
Are you sure you want to change the base?
Conversation
…or in websocket iam auth
…or in websocket iam auth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter fails with the following errors:
❌ The title of the pull request should omit 'aws-' from the name of modified packages. Use 'apigatewayv2' instead of 'aws-apigatewayv2'.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed, add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
For clarity, "argument stage" mentioned is not the specific issue, stage in the old method was the 3rd argument, therefore this error should be interpreted as, breaking change due to argument count change. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Same error, I merged to invoke a retry just in case. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33100 +/- ##
=======================================
Coverage 81.54% 81.54%
=======================================
Files 226 226
Lines 13777 13777
Branches 2414 2414
=======================================
Hits 11235 11235
Misses 2270 2270
Partials 272 272
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Not opening issue since it would likely be P2 anyways.
Reason for this change
In websocket APIs in
aws-apigatewayv2
, the function arnForExecuteApi has essentially the same exact functionality as a REST API, which is not appropriate for Websockets which are fundamentally different.The way I found this issue was I used arnForExecuteApi to put the arn of the api into an IAM Role. The reason for this was because I was trying to use an AIM authorizer, which from a React standpoint meant signing iam credentials from my Cognito id pool using Amplify lib. When doing this I used arnForExecuteApi from CDK to write the policy, I did not include any arguments, just the default.
The issue was that this was not working. I spent time diving deep on the issue in case it was the method in which I was signing the credentials, since I was not too familiar with this process. I also got the assistance of a Cloud Support Engineer from AWS to try and identify the problem.
Shout-out Mike Sacks.
The problem ended up being that that the resource policy was not correct. The policy that was generated by the function arnForExecuteApi was
This is because the function itself has 3 values, stage, method and path, so when all are left in default states, this indicates
all
or*
. So when adding each value at default you get/*/*/*
, 3 x /*.This is an issue because Websocket arns are not structured like this, and as it turns out iam prevents access if you have too many wild cards than applicable. This means the reason for getting access denied was not because of my signed url, but because having 1 extra /* means that you no longer have permissions.
Websocket arns are structured like this
In this example, * is the stage (this is what it shows on the console) and $connect is the
route
.You can add as many routes as you want, but the main ones by default are $connect, $disconnect and $default for no matching route. So if I want to grant an IAM role to have access to all routes and all stages, I would use this:
Note 2 x /* instead of 3.
Simply changing this by hand (deleting 2 characters) was enough to get the websocket to begin connecting via my signed url.
Description of changes
A re-write of the function for creating the arn
I removed "Method" and "Path" entirely since these are not even appropriate to use as terms for websockets. I used Route instead.
Description of how you validated changes
Updated Tests, there were 4 tests before:
$
to check that the$
is being added correctly.$
at the front, but since I baked this into the function to check if there is one or not, this test is not needed.This leaves 2 total tests now.
Added a new integ function,
integ.api-grant-invoke.ts
and used --update-on-failed with my personal account to bootstrap new snapshots to match. For this test I included an iam role and 2 arns, one with default settings and one with.arnForExecuteApi('connect', 'prod')
Intentionally left off the
$
to check that it's being added.All tests and integ are passing.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license